Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

docs(directive): Clarified and cleaned up directive guide #2888

Closed
wants to merge 1 commit into from
Closed

docs(directive): Clarified and cleaned up directive guide #2888

wants to merge 1 commit into from

Conversation

ProLoser
Copy link
Contributor

@ProLoser ProLoser commented Jun 5, 2013

  • corrected terminology about how directives use require
  • added more variations to the DirectiveDefinitionObject
  • removed some slightly superfluous text

@@ -63,7 +63,7 @@ api/ng.$rootScope.Scope#$digest digest} cycle. An example of interpolation is sh
here:

<pre>
<a href="img/{{username}}.jpg">Hello {{username}}!</a>
<a ng-href="img/{{username}}.jpg">Hello {{username}}!</a>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

@petebacondarwin
Copy link
Contributor

@ProLoser - can you take a look at my comments and rework this PR? Thanks.

* `?` - Don't raise an error. This makes the require dependency optional.
* `^` - Look for the controller on parent elements as well.
* `?` - Required dependency is optional. The parameter might be `null/undefined`.
* `^` - Look for the directive on parent/ancestor elements as well.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the ^? case should also be explicitly added here as (to me) it wasn't clear that both modifier could be specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would actually add a note above or below the bulleted items and not as an explicit item in the list. However re-reading my description, I think "Required dependency is optional" couldn't be any more confusing lol.

@petebacondarwin
Copy link
Contributor

@ProLoser - is this ready to be reviewed now?

@petebacondarwin
Copy link
Contributor

@ProLoser - surely you have signed the CLA at some point?

@petebacondarwin
Copy link
Contributor

PR Checklist (Docs-only fix)

  • PR is rebased against recent master
  • PR is squashed into one commit per logical change
  • PR's commit messages are descriptive and allows us to autogenerate release notes (required commit message format)
  • All changes requested in review have been implemented

@ProLoser
Copy link
Contributor Author

Rebased.

- corrected terminology about how directives use `require`
- added more variations to the DirectiveDefinitionObject
- removed some slightly superfluous text

docs(directive): Minor correction to example to avoid bad practice

Anchor tags should use `ng-href` instead of `href` for interpolation.

docs(directive): Supplementing DDO description

DDO = Directive Definition Object
Tweak recommended here:
https://github.com/angular/angular.js/pull/2888/files#r4664565
@ProLoser
Copy link
Contributor Author

Squashed

@jeffbcross jeffbcross closed this Jul 24, 2013
@petebacondarwin
Copy link
Contributor

As 454bcfa

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants